feat(action-popover): use subtle Action trigger and deprecate positio… - #8137
feat(action-popover): use subtle Action trigger and deprecate positio…#8137tamas-sage wants to merge 4 commits into
Conversation
|
We should be using the PopoverMenu to replace the existing menu, you may need to do some mapping/aliasing to get it to work, happy to chat about it on slack if needed |
|
|
||
| // Any printable character: focus the next item whose label starts with that | ||
| // character, wrapping to the start of the list when there is no later match | ||
| if ( |
There was a problem hiding this comment.
suggestion: the length checks could be combined to
if (
isButtonMenu &&
ev.key.trim().length === 1 &&
!ev.ctrlKey &&
!ev.metaKey &&
!ev.altKey &&
)
There was a problem hiding this comment.
After discussing with @Fenwick17 I think we need to implement this slightly differently as the behaviour isn't intended for SplitButton or MultiActionButton but they will now have it etc. I was thinking we could add a typeahead prop that lets you pass the above as a callback via ActionPopover as a form of opt-in
<PopoverMenu
isButtonMenu
typeahead={handleAlphaKeyNavigation}
/>
| export { default as ActionPopoverMenuButton } from "./action-popover-menu-button/action-popover-menu-button.component"; | ||
| /** @deprecated Use `renderButton` with your own button component instead. */ | ||
| export type { ActionPopoverMenuButtonProps } from "./action-popover-menu-button/action-popover-menu-button.component"; | ||
| export { default as ActionPopoverDivider } from "./action-popover-divider/action-popover-divider.component"; |
There was a problem hiding this comment.
comment: the component has been deleted from this path
|
|
||
| &[aria-expanded="true"] { | ||
| ${addFocusStyling()} | ||
| background-color: var(--colorsActionMajorTransparent); |
There was a problem hiding this comment.
question: is it correct we're using the old tokens here?
| @@ -5,7 +5,6 @@ import { | |||
| ActionPopoverDivider, | |||
There was a problem hiding this comment.
comment: wrong path for the divider (same in test and interaction stories as well)
| onSubmenuOpen?: () => void; | ||
| onSubmenuClose?: () => void; | ||
| /** Fired when the pointer enters the item, used to open submenus on hover */ | ||
| onMouseEnter?: (event: React.MouseEvent<HTMLLIElement>) => void; |
There was a problem hiding this comment.
comment: I think it we're okay to drop this behaviour as it's not typical or part of the button menu pattern
cc @Fenwick17
| ); | ||
|
|
||
| const interactiveElement = href ? ( | ||
| <a |
There was a problem hiding this comment.
comment: we'll need to override the user agent styles here, we might be able to just use Button (next) as it supports being rendered as an anchor element etc
| listRef={menu} | ||
| listboxAriaLabelledBy={parentID} | ||
| id={menuID} | ||
| disableBackgroundUI={isInFlatTable} |
There was a problem hiding this comment.
question: do we need this? I'm not sure where the requirement for this has come from
There was a problem hiding this comment.
comment: We'd need this exported via the original directory (action-popover-divider/action-popover-divider.component) structure or this would technically be a breaking change
| <Button | ||
| variant="default" | ||
| variantType="subtle" | ||
| iconType="dropdown" |
There was a problem hiding this comment.
comment: it looks like the icon should be ellipsis in the designs
2 participants
Proposed behaviour
Use a subtle Action button with a three-dots icon, keep it outlined while open, deprecate manual positioning props, and rely on automatic flipping with docs/stories showing text and textless render-prop examples.
Current behaviour
Most of this is implemented (subtle Action button, open-state outline, deprecations, and examples), but the trigger icon is still dropdown in the component/stories instead of three dots.
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions